Collections: Customizable collection names#878
Conversation
📝 WalkthroughWalkthroughAdds PATCH /collections/{collection_id} using a new CollectionUpdate model; extends CollectionPublic with optional name and description; updates to_collection_public to include those fields; implements CollectionCrud.update with name-uniqueness and IntegrityError handling; adds docs, re-export, and API/CRUD tests. ChangesCollection update flow
sequenceDiagram
participant Client
participant API as update_collection route
participant Crud as CollectionCrud.update
participant DB as Database
participant Helper as to_collection_public
Client->>API: PATCH /collections/{id} with CollectionUpdate
API->>Crud: CollectionCrud.update(collection_id, patch)
Crud->>DB: load collection / persist changes
Crud->>Helper: to_collection_public(updated_collection)
Helper->>API: CollectionPublic
API->>Client: 200 OK with CollectionPublic
🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/routes/collections.py`:
- Around line 211-234: The update_collection function is missing an explicit
return type; add a return type hint (e.g., -> APIResponse) to the
update_collection signature to reflect that it returns
APIResponse.success_response(...), and if APIResponse is not already imported in
backend/app/api/routes/collections.py, add the appropriate import; keep the
existing parameter annotations and ensure the signature uses the exact function
name update_collection and the same parameters (session: SessionDep,
current_user: AuthContextDep, patch: CollectionUpdate, collection_id: UUID =
FastPath(...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6747a73-5f99-43f4-8f5a-0f72b5e39d52
📒 Files selected for processing (5)
backend/app/api/docs/collections/update.mdbackend/app/api/routes/collections.pybackend/app/crud/collection/collection.pybackend/app/models/__init__.pybackend/app/models/collection.py
✅ Files skipped from review due to trivial changes (1)
- backend/app/api/docs/collections/update.md
| description: str | None = Field( | ||
| default=None, | ||
| description="Description of the collection", | ||
| ) |
There was a problem hiding this comment.
can you add testcases for this as well
There was a problem hiding this comment.
Yes, added the testcases.
|
|
||
| class CollectionPublic(SQLModel): | ||
| id: UUID | ||
| name: str | None = Field( |
There was a problem hiding this comment.
while this is fine can we add some restriction like name should be there instead of name and minimum and maximum length
There was a problem hiding this comment.
Update the code accordingly.
| raise HTTPException( | ||
| status_code=409, | ||
| detail=f"Collection '{changes['name']}' already exists. Choose a different name.", | ||
| ) |
There was a problem hiding this comment.
can you check other code, usually HTTPException belongs in routes, not crud/
There was a problem hiding this comment.
Please check this again.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/tests/api/routes/collections/test_collection_update.py (1)
13-91: ⚖️ Poor tradeoffConsider using factory pattern for test data creation.
Similar to the CRUD tests, these API tests use helper functions
get_project()andget_assistant_collection()rather than the factory pattern. As per coding guidelines, test fixtures inbackend/app/tests/should use the factory pattern.As per coding guidelines: Use factory pattern for test fixtures in
backend/app/tests/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/tests/api/routes/collections/test_collection_update.py` around lines 13 - 91, These API tests (test_update_collection_returns_updated_fields, test_update_collection_partial_update_preserves_other_fields, test_update_collection_rename_to_existing_name_returns_409, test_update_collection_not_found_returns_404) use helper functions get_project() and get_assistant_collection(); update them to follow the repository guideline by using the factory pattern instead: import and use the appropriate ProjectFactory and CollectionFactory (or equivalent factories used in CRUD tests) to create projects and collections in each test, replace calls to get_project() and get_assistant_collection() with factory.create(...) (or the factory's create/ build API), and remove reliance on those helpers so fixtures align with backend/app/tests/ conventions while keeping the same test assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/tests/api/routes/collections/test_collection_update.py`:
- Around line 13-91: These API tests
(test_update_collection_returns_updated_fields,
test_update_collection_partial_update_preserves_other_fields,
test_update_collection_rename_to_existing_name_returns_409,
test_update_collection_not_found_returns_404) use helper functions get_project()
and get_assistant_collection(); update them to follow the repository guideline
by using the factory pattern instead: import and use the appropriate
ProjectFactory and CollectionFactory (or equivalent factories used in CRUD
tests) to create projects and collections in each test, replace calls to
get_project() and get_assistant_collection() with factory.create(...) (or the
factory's create/ build API), and remove reliance on those helpers so fixtures
align with backend/app/tests/ conventions while keeping the same test
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 664e3b2c-41f7-4cc8-ace1-81b6d3b4ea31
📒 Files selected for processing (4)
backend/app/api/routes/collections.pybackend/app/crud/collection/collection.pybackend/app/tests/api/routes/collections/test_collection_update.pybackend/app/tests/crud/collections/collection/test_crud_collection_update.py
| collection_crud = CollectionCrud(session, current_user.project_.id) | ||
| try: | ||
| collection = collection_crud.update(collection_id, patch) | ||
| except CollectionNameConflictError as err: |
There was a problem hiding this comment.
(Nitpick) If there is an error at the crud layer we can raise the httpexception at crud file only .. no need of raising httpexception here..
There was a problem hiding this comment.
Check this comment #878 (comment). Earlier, I added the httpexception at crub file. But then I change the code according to comment.
Issue: ProjectTech4DevAI/kaapi-frontend#172
Summary
nameanddescriptionfields, providing more detailed collection information in public endpoints.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.